Skip to content

libnixf: check builtins. prefix from variable lookups#721

Merged
inclyc merged 1 commit into
mainfrom
libnixf/checkout-builtins
Oct 6, 2025
Merged

libnixf: check builtins. prefix from variable lookups#721
inclyc merged 1 commit into
mainfrom
libnixf/checkout-builtins

Conversation

@inclyc

@inclyc inclyc commented Oct 6, 2025

Copy link
Copy Markdown
Member

No description provided.

Base automatically changed from builtins-prefix to main October 6, 2025 18:18
@inclyc inclyc requested a review from Ruixi-rebirth as a code owner October 6, 2025 18:18
@inclyc inclyc force-pushed the libnixf/checkout-builtins branch from 496fae8 to 46a12cc Compare October 6, 2025 18:19
@inclyc inclyc changed the title libnixf: check 'builtins.' prefix from variable lookups libnixf: check builtins. prefix from variable lookups Oct 6, 2025
@inclyc inclyc merged commit 77c185a into main Oct 6, 2025
19 checks passed
@inclyc inclyc deleted the libnixf/checkout-builtins branch October 6, 2025 18:28
@philiptaron

Copy link
Copy Markdown

Is this correct? The list of top-level names isn't everything in builtins, it's only this set:

  1. abort
  2. baseNameOf
  3. break
  4. derivation
  5. derivationStrict
  6. dirOf
  7. false
  8. fetchGit
  9. fetchMercurial
  10. fetchTarball
  11. fetchTree
  12. fromTOML
  13. import
  14. isNull
  15. map
  16. null
  17. placeholder
  18. removeAttrs
  19. scopedImport
  20. throw
  21. toString
  22. true

I don't see any gating in this PR to just those names.

@inclyc

inclyc commented Oct 6, 2025

Copy link
Copy Markdown
Member Author

Is this correct? The list of top-level names isn't everything in builtins, it's only this set:

  1. abort
  2. baseNameOf
  3. break
  4. derivation
  5. derivationStrict
  6. dirOf
  7. false
  8. fetchGit
  9. fetchMercurial
  10. fetchTarball
  11. fetchTree
  12. fromTOML
  13. import
  14. isNull
  15. map
  16. null
  17. placeholder
  18. removeAttrs
  19. scopedImport
  20. throw
  21. toString
  22. true

I don't see any gating in this PR to just those names.

That list is generated by the NixOS/nix library. Specifically, those values are referred to as PrimOp in that implementation. PrimOps follow these visibility rules:

  1. __xxx -> builtins.xxx + __xxx
  2. xxx -> builtins.xxx + xxx

This information is obtained directly from the C++ library and is not encoded in the nixd code.

@inclyc

inclyc commented Oct 6, 2025

Copy link
Copy Markdown
Member Author
builtins_480p.mov

@philiptaron See this video, is that expected?

@philiptaron

Copy link
Copy Markdown

This information is obtained directly from the C++ library and is not encoded in the nixd code.

Wonderful.

@MattSturgeon

MattSturgeon commented Oct 6, 2025

Copy link
Copy Markdown
Member

This information is obtained directly from the C++ library and is not encoded in the nixd code.

My only concern with this approach is when using this for nixpkgs specifically, is nixpkgs is supposed to eval on multiple implementations & versions of nix.

E.g. if a new version of nix adds or remove something from the prelude, nixpkgs wouldn't want to change its linting rules until that change was reflected in all "supported" nix versions (i.e. when we bump minFeatures to include the change).

I wonder if there is a way to configure this linting and use the C++ information merely as the default? E.g. we could have an --enforce-globals list:

--enforce-global true \
--enforce-global false \
--enforce-global map \
--enforce-global throw \
# ...

When used, only the explicitly listed "globals" would be treated as prelude values.

IDK if such an interface would need a way to both append to the default list or override it? For nixpkgs we'd want to override the list, but maybe others would want a different behaviour?

@philiptaron

Copy link
Copy Markdown

I would be shocked if Nix introduced a new name to the prelude; they've almost always been added to builtins.

@MattSturgeon

Copy link
Copy Markdown
Member

I would be shocked if Nix introduced a new name to the prelude; they've almost always been added to builtins.

I don't know how comfortable I am relying on that assumption, when the result of something being added/moved/removed would result in linting errors across nixpkgs and fixing those lint errors would break eval on older still-supported versions (and other supported implementations).

I guess the questions are 1) when did the prelude last change? 2) are there any plans to move things into or out of the prelude? That would give us an idea of how unreasonably my discomfort is 😁

@trueNAHO

trueNAHO commented Oct 7, 2025

Copy link
Copy Markdown
Member

I would be shocked if Nix introduced a new name to the prelude; they've almost always been added to builtins.

I don't know how comfortable I am relying on that assumption, when the result of something being added/moved/removed would result in linting errors across nixpkgs and fixing those lint errors would break eval on older still-supported versions (and other supported implementations).

I guess the questions are 1) when did the prelude last change? 2) are there any plans to move things into or out of the prelude? That would give us an idea of how unreasonably my discomfort is 😁

That is more or less what NixOS/nixpkgs#449123 (comment) is trying to figure out.

@wolfgangwalther

Copy link
Copy Markdown

To avoid problems with later nix versions adding new globals - how about we achieve consistency by enforcing builtins. prefixes to all of these?

This does not suffer from the same problem. It is also much better to feature detect for lib/minfeatures.nix in Nixpkgs.

Ultimately, we just want consistency and nothing to nit-pick about, right?

@inclyc

inclyc commented Oct 7, 2025

Copy link
Copy Markdown
Member Author

To avoid problems with later nix versions adding new globals - how about we achieve consistency by enforcing builtins. prefixes to all of these?

This does not suffer from the same problem. It is also much better to feature detect for lib/minfeatures.nix in Nixpkgs.

Ultimately, we just want consistency and nothing to nit-pick about, right?

Too pedantic to write builtins.true ?

@wolfgangwalther

Copy link
Copy Markdown

In that case, I agree with @MattSturgeon that it would be best to freeze this list at those globals that were present in 2.18.

@eclairevoyant

eclairevoyant commented Oct 7, 2025

Copy link
Copy Markdown

I guess the questions are 1) when did the prelude last change?

Every globally-exposed primop (not prefixed with __) has been present since nix 2.3, except for

@MattSturgeon

Copy link
Copy Markdown
Member

I understand if this is considered not worth it, currently. But in an ideal world, I'd like to see --exclude-prelude and --include-prelude.

--exclude-prelude would allow making specific exceptions to the rule. E.g. --exclude-prelude fromTOML would enforce builtins.fromTOML instead of fromTOML for consistency with builtins.fromJSON.

--include-prelude would allow overriding the rule with an explicit list. E.g. --include-prelude true --include-prelude false would act as if true and false are the only prelude items. When used, there would be no need to check nix's actual preludes.

While I think this would be useful beyond nixpkgs, it would also allow nixpkgs to handle backwards compatibility if anything does get added to the prelude in the future. Based on @eclairevoyant's comment this seems infrequent and unlikely, but still possible.

Personally, I think --exclude-prelude seems more useful generally. --include-prelude is kinda niche and, I assume, more effort to implement. It'd only be useful for projects that want to use a strict subset of the prelude.

@eclairevoyant

Copy link
Copy Markdown

Side note: "prelude" isn't a documented term anywhere in nix, I think it's a haskell thing.
(Though, not trying to get too in the weeds on naming, before deciding if it's even worth it obviously.)

@inclyc inclyc added the enhancement New feature or request label Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants